Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tmc integration test - Wait for shiny app idle after fit is clicked #1296

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Dec 10, 2024

Potentially closes #1258

Potentially, because I was not able to reproduce this issue in any way to know for sure that fixes the issue. I tested in macOS, linux (docker image our CI uses), and Windows machines after installing all the packages from the GitHub main branch and was unable to reproduce this behavior.

We can close the issue after we confirm the daily integration test is resolved.

Rationale for the change:
Since this module only works after the Fit Model button is clicked the shinytest2 logic goes as follows:

  1. Initialize the TealAppDriver object using the tm_a_mmrm() module.
  2. Click the Fit Model button.
  3. Perform the test logic:
    a. Select the Output Type
    b. Check for errors
    c. Check if output changed

image

I think that when we start checking for errors we spot the validation error before the output had time to render because the output type we want to select is in the first selection, which is pre-selected, and since we do not wait for input changes when the input is the same, we quickly start checking for errors and observe the validation error.
If, this is the case, this PR will fix it.

@vedhav vedhav added bug Something isn't working core labels Dec 10, 2024
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Unit Tests Summary

    1 files     70 suites   1h 8m 52s ⏱️
  724 tests   614 ✅ 110 💤 0 ❌
1 984 runs  1 759 ✅ 225 💤 0 ❌

Results for commit a2da0c5.

♻️ This comment has been updated with latest results.

Copy link
Contributor

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-tm_a_mmrm 💔 $722.91$ $+17.76$ $0$ $0$ $0$ $0$
shinytest2-tm_g_barchart_simple 💚 $225.00$ $-2.14$ $0$ $0$ $0$ $0$
shinytest2-tm_g_ci 💚 $97.92$ $-1.50$ $0$ $0$ $0$ $0$
shinytest2-tm_g_km 💚 $262.95$ $-2.97$ $0$ $0$ $0$ $0$
shinytest2-tm_g_pp_adverse_events 💔 $119.75$ $+3.24$ $0$ $0$ $0$ $0$
shinytest2-tm_g_pp_patient_timeline 💔 $241.76$ $+2.15$ $0$ $0$ $0$ $0$
shinytest2-tm_g_pp_therapy 💔 $189.23$ $+3.18$ $0$ $0$ $0$ $0$
shinytest2-tm_t_summary_by 💔 $78.99$ $+1.07$ $0$ $0$ $0$ $0$
shinytest2-tm_t_tte 💔 $66.14$ $+1.86$ $0$ $0$ $0$ $0$

Results for commit bd86cae

♻️ This comment has been updated with latest results.

@averissimo averissimo self-assigned this Dec 10, 2024
Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good change.

This module has some bugs with active issues on the backlog, we might need to skip these tests until those are solved

tests/testthat/test-shinytest2-tm_a_mmrm.R Show resolved Hide resolved
@vedhav vedhav merged commit b609be2 into main Dec 10, 2024
26 checks passed
@vedhav vedhav deleted the 1258-fix-test@main branch December 10, 2024 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: test failure on tm_a_mmrm; message: Inputs changed and no longer reflect the fitted model.
2 participants